[Feature] Sign rework api (part 1 of 2 for sign rework PR) #706

DerToaster98 · 2024-08-31T15:23:34Z

Describe in detail what your pull request accomplishes

As discussed with @oh-noey the original PR (#679 ) is being splitted into two parts.
The first part (this PR) being the necessary API classes and changes ONLY.

That contains the base classes for all signs as well as their registration and the sign wrapper shenanigans.

This does not yet contain the necessary changes to TranslationTask.java and RotationTask.java as those changes need to be looked at again. Most of the change is switching to a call to the signListener instance though.

Checklist

Tested
Performance tested (Could improve performance, but mainly makes the sign behaviors extendalbe and overridable in a nicer way. Also reduces duplicate code)

(cherry picked from commit 5248bc2) (cherry picked from commit 960b30a)

(cherry picked from commit 7f46895) (cherry picked from commit 39e08a5)

(cherry picked from commit d5ded2e) (cherry picked from commit 9b7ea60)

(cherry picked from commit aff2a1d) (cherry picked from commit 48a9bc1)

(cherry picked from commit dc85f9d) (cherry picked from commit ca35cd3)

(cherry picked from commit 464fcd6)

…again (cherry picked from commit a307803) (cherry picked from commit b65562e)

(cherry picked from commit 132f179) (cherry picked from commit 453b0a5)

(cherry picked from commit ce7f379)

(cherry picked from commit 1aaf852)

(cherry picked from commit d70b65b)

(cherry picked from commit e2bae7d)

(cherry picked from commit d86b510)

(cherry picked from commit 3194ed7)

(cherry picked from commit 2df0385)

TylerS1066

I did an initial look through, to properly understand the AbstractMovecraftSign I think I'll need to try my hand at updating a sign or two.

TylerS1066 · 2024-09-08T15:30:01Z

api/src/main/java/net/countercraft/movecraft/sign/AbstractMovecraftSign.java

+// DONE: In 1.21 signs can have multiple sides! This requires us to pass the clicked side through or well the relevant lines and the set method for the clicked side => Resolved using the SignWrapper
+/*


Does this mean a two-sided sign effectively acts like two signs to Movecraft?

Yep, each side can have different text, which makes each side act as individual side.

So basically it would be like putting two one sided signs with their backs to each other.

I would like to keep it like this, it's useful for hanging cruise signs or for peripheral stuff like contacts and Status

TylerS1066 · 2024-09-08T15:31:02Z

api/src/main/java/net/countercraft/movecraft/sign/AbstractMovecraftSign.java

+ *  - SignChangeEvent
+ *  - PlayerInteractEvent, if the clicked block is a sign
+ *  - CraftDetectEvent
+ *  - SignTranslateEvent (if the sign is a subclass of AbstractCraftSign)


The overhead of the Bukkit event system is significant when translating signs. Is it possible that this system could replace it?

Ya, i could change it so it has a "onTranslate" function in the base class. I'd still make it collect "all the same signs" per type and then calling their "instance", so in effect what it does right now, but without the bukkit event being the "bridge".

api/src/main/java/net/countercraft/movecraft/sign/AbstractMovecraftSign.java

api/src/main/java/net/countercraft/movecraft/sign/AbstractCraftSign.java

TylerS1066 · 2024-09-08T16:06:13Z

api/src/main/java/net/countercraft/movecraft/sign/AbstractCraftSign.java

+    public void onCraftDetect(CraftDetectEvent event, AbstractSignListener.SignWrapper sign) {
+        // Do nothing by default
+    }
+
+    public void onSignMovedByCraft(SignTranslateEvent event) {
+        // Do nothing by default
+    }


Do we need these?

Those implementations? Not really, but the functiosn should be there as at least abstract. I use the CraftDetectEvent callback to handle stuff like setting the name, displaying the Status and so on on mount (latter is not fully working yet but eventually)

The other is just the callback for when the sign was moved by the craft, updating stuff and such.

I will leave them in as that for now. Not all craft signs require them but we should give the opportunity to react to that

api/src/main/java/net/countercraft/movecraft/sign/AbstractMovecraftSign.java

TylerS1066 · 2024-09-08T21:14:05Z

Movecraft/src/main/java/net/countercraft/movecraft/sign/CraftPilotSign.java

+                        /*if (signWrapper.block().getBlockData() instanceof Directional)
                            craft.setCruiseDirection(CruiseDirection.fromBlockFace(((Directional) sign.getBlockData()).getFacing()));
                        else
-                            craft.setCruiseDirection(CruiseDirection.NONE);
+                            craft.setCruiseDirection(CruiseDirection.NONE);*/


We can remove commented out code, Git keeps it around.

sure, will do that when i work on the fork again

api/src/main/java/net/countercraft/movecraft/events/SignTranslateEvent.java

DerToaster98 · 2024-09-09T16:35:51Z

I did an initial look through, to properly understand the AbstractMovecraftSign I think I'll need to try my hand at updating a sign or two.

For examples on the implementations you can just check the (older) PR which also has the implementations (#679 ) or the testbranch in my fork

oh-noey · 2024-09-09T17:30:50Z

api/src/main/java/net/countercraft/movecraft/sign/AbstractMovecraftSign.java

+ */
+public abstract class AbstractMovecraftSign {
+
+    private static final Map<String, AbstractMovecraftSign> SIGNS = Collections.synchronizedMap(new HashMap<>());


Can we move this whole registry system outside of the actual AbstractMovecraftSign implementation, and use a singleton instead of static? Perhaps a MovecraftSignRegistry? we can make it accessible from the Movecraft instance for now. With our move towards DI/inversion of control, we should avoid adding new static singletons like this.

Once it's merged. We already have some registry thing used in the DataTags topic, so if we add a generic registry implemtnation (i have one somewhere) we should adjust that too

I dont think a generic registry is a good idea - its just a naming convention. I just would prefer to seperate the registration system from the registrars

I meant a generic registry base class

Added a small generic registry base class that can be used for multiple registries, does it fit your needs?

(cherry picked from commit 91ff872)

(cherry picked from commit 633f33c)

(cherry picked from commit 88baa63)

(cherry picked from commit fd70b50)

DerToaster98 · 2024-09-10T18:14:40Z

Adressed your points

(cherry picked from commit 54336e5)

(cherry picked from commit 27b9491)

(cherry picked from commit f7fd65e)

(cherry picked from commit 6a09a70)

(cherry picked from commit 8e7f1bf)

(cherry picked from commit 08b4e6d)

(cherry picked from commit e3a730c)

(cherry picked from commit a426bcf)

(cherry picked from commit 869cb5a)

(cherry picked from commit c345a25)

(cherry picked from commit 834794f)

(cherry picked from commit 2454f78)

(cherry picked from commit dc459d2)

(cherry picked from commit 77bb394)

(cherry picked from commit 397e7fe)

(cherry picked from commit 441452c)

(cherry picked from commit 0433e31)

(cherry picked from commit 0ce7710)

DerToaster98 · 2024-09-17T16:30:57Z

PR is ready for new review, it is now at the same state as my own testing branch which is being run since 2 weeks on a different server and so far works good

(cherry picked from commit c83b41e)

(cherry picked from commit 64800b8)

DerToaster98 added 23 commits August 31, 2024 17:08

sign rework - add API classes

a8a44db

utility method to get Rotation by action

766cb8c

necessary changes to SignTranslateEvent => use components (basically...)

2074d9d

signListener implementations

e4292f1

add instance for sign listener

301b0bf

add base signs (most basic signs that exist)

ff84ee8

forgot action import

7107633

fix getSignWrappers() method for sign => NPE

df4d4ef

(cherry picked from commit 5248bc2) (cherry picked from commit 960b30a)

corrections to BlockFace access

f9e1ff6

(cherry picked from commit 7f46895) (cherry picked from commit 39e08a5)

more fixes regarding getting the blockface of a sign...

b6bbdab

(cherry picked from commit d5ded2e) (cherry picked from commit 9b7ea60)

NPE prevention

674c132

(cherry picked from commit aff2a1d) (cherry picked from commit 48a9bc1)

add TODO note

cb19e75

(cherry picked from commit dc85f9d) (cherry picked from commit ca35cd3)

remove spaces from suffix

bf4c7c9

(cherry picked from commit 464fcd6)

Startup correction cherry picked

8422b22

correction => if the ident has trailing : => don't add them yourself …

221d0f2

…again (cherry picked from commit a307803) (cherry picked from commit b65562e)

startup correction

1e60c83

(cherry picked from commit 132f179) (cherry picked from commit 453b0a5)

forgot the space

719a1ec

(cherry picked from commit ce7f379)

crafttype is specified in second, not the third line

956c8c5

(cherry picked from commit 1aaf852)

typo

8e1e7c7

(cherry picked from commit d70b65b)

fix information signs not updating

4eda66f

(cherry picked from commit e2bae7d)

comment out debug logs

d9afa5f

(cherry picked from commit d86b510)

call update on the state, not on the block...

19e2171

(cherry picked from commit 3194ed7)

more elegant solution

6d8b47a

(cherry picked from commit 2df0385)

TylerS1066 reviewed Sep 8, 2024

View reviewed changes

oh-noey reviewed Sep 9, 2024

View reviewed changes

DerToaster98 added 4 commits September 10, 2024 20:08

add eventtype enum (to api classes)

94ccf3c

(cherry picked from commit 91ff872)

refactor for eventtype enum (implementations (simple))

0a7fd09

(cherry picked from commit 633f33c)

supply eventType and use nullable for sign get access

a611a4f

just call it override

be724f9

(cherry picked from commit 88baa63)

remove old code

17b458e

(cherry picked from commit fd70b50)

DerToaster98 requested a review from TylerS1066 September 10, 2024 18:14

DerToaster98 added 22 commits September 11, 2024 18:46

remove 1.18 signListener

4a6941a

(cherry picked from commit 54336e5)

fix build errors

000d987

properly implement shouldCancel method (API + examples)

8349dd8

(cherry picked from commit 27b9491)

add override items and tools

e0f09d1

(cherry picked from commit f7fd65e)

small tweaks regarding interactions

c68ff55

(cherry picked from commit 6a09a70)

NPE prevention

e4ecbf9

(cherry picked from commit 8e7f1bf)

respect edits with dyecolors, wax, etc

6ab1baa

(cherry picked from commit 08b4e6d)

extract base elements from Cruise sign into a toggle sign

4a1f985

(cherry picked from commit e3a730c)

update signs after detect

b697375

(cherry picked from commit a426bcf)

cancel sign clicks on crafts in all cases

b4f9032

(cherry picked from commit 869cb5a)

only cancel when sign is empty

35ea71e

(cherry picked from commit c345a25)

fix override param

9653b43

(cherry picked from commit 834794f)

fix subcraft sign creation

78d19f3

(cherry picked from commit 2454f78)

Merge branch 'APDevTeam:main' into sign-rework-api

a4e21ec

Remote version specific SignListener implementations

889a5cf

adapt to refactoring

754f22e

(cherry picked from commit dc459d2)

use newer method

ff1fd2f

(cherry picked from commit 77bb394)

fix compile errors and remove old code

ab2ebb7

apply default text and implement processSignChange

8c14dda

(cherry picked from commit 397e7fe)

correct typo

e864150

(cherry picked from commit 441452c)

return true

8f9ee61

(cherry picked from commit 0433e31)

call stuff for the other side, if that side is a movecraft sign

c722fbb

(cherry picked from commit 0ce7710)

DerToaster98 requested a review from oh-noey September 17, 2024 16:31

DerToaster98 added 3 commits September 17, 2024 18:33

fix compile errors

ab7ae64

add getter for crafttype

9d850ce

(cherry picked from commit c83b41e)

check if player actually is the pilot of that craft

9d35183

(cherry picked from commit 64800b8)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Sign rework api (part 1 of 2 for sign rework PR) #706

[Feature] Sign rework api (part 1 of 2 for sign rework PR) #706

DerToaster98 commented Aug 31, 2024

TylerS1066 left a comment

TylerS1066 Sep 8, 2024

DerToaster98 Sep 9, 2024

TylerS1066 Sep 8, 2024

DerToaster98 Sep 9, 2024

TylerS1066 Sep 8, 2024

DerToaster98 Sep 9, 2024

DerToaster98 Sep 10, 2024

TylerS1066 Sep 8, 2024

DerToaster98 Sep 9, 2024

DerToaster98 commented Sep 9, 2024

oh-noey Sep 9, 2024 •

edited

Loading

DerToaster98 Sep 9, 2024

oh-noey Sep 9, 2024

DerToaster98 Sep 9, 2024

DerToaster98 Sep 10, 2024

DerToaster98 commented Sep 10, 2024

DerToaster98 commented Sep 17, 2024

		// DONE: In 1.21 signs can have multiple sides! This requires us to pass the clicked side through or well the relevant lines and the set method for the clicked side => Resolved using the SignWrapper
		/*

[Feature] Sign rework api (part 1 of 2 for sign rework PR) #706

Are you sure you want to change the base?

[Feature] Sign rework api (part 1 of 2 for sign rework PR) #706

Conversation

DerToaster98 commented Aug 31, 2024

Describe in detail what your pull request accomplishes

Checklist

TylerS1066 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerToaster98 commented Sep 9, 2024

oh-noey Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerToaster98 commented Sep 10, 2024

DerToaster98 commented Sep 17, 2024

oh-noey Sep 9, 2024 •

edited

Loading